Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix][ml] Avoid NPE when getCurrentLedger() returns null #24058

Closed
wants to merge 1 commit into from
Closed

[fix][ml] Avoid NPE when getCurrentLedger() returns null #24058

wants to merge 1 commit into from

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Mar 5, 2025

Motivation

In branch-3.0, tests are failing with logs about NPEs such as

  java.lang.NullPointerException: Cannot invoke "org.apache.bookkeeper.client.LedgerHandle.getId()" because the return value of "org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.getCurrentLedger()" is null
  	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.estimateEntryCountBySize(ManagedCursorImpl.java:3705)
  	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.applyMaxSizeCap(ManagedCursorImpl.java:3691)
  	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReadEntriesWithSkip(ManagedCursorImpl.java:853)
  	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReadEntries(ManagedCursorImpl.java:840)

This is due to #23931 changes.

Modifications

Handle the case when getCurrentLedger() returns null

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 4.1.0 milestone Mar 5, 2025
@lhotari lhotari self-assigned this Mar 5, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 5, 2025
@lhotari lhotari mentioned this pull request Mar 5, 2025
Open
3 tasks
poorbarcode
poorbarcode reviewed Mar 5, 2025
View reviewed changes
Copy link
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am considering why managedLedger.currentLedger will be null, could you explain the test's name?

@lhotari
Copy link
Member Author

lhotari commented Mar 5, 2025

@poorbarcode In branch-3.0, there are at least 2 tests where this can be seen in the output.
org.apache.pulsar.broker.service.ClusterMigrationTest#testClusterMigration is one. The issue is #24038

@lhotari
Copy link
Member Author

lhotari commented Mar 5, 2025

It seems that currentLedger could be null immediately after the ledger has been created, but not after that.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.19%. Comparing base (bbc6224) to head (09c29bc).
Report is 948 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24058      +/-   ##
============================================
+ Coverage     73.57%   74.19%   +0.62%     
+ Complexity    32624    32405     -219     
============================================
  Files          1877     1861      -16     
  Lines        139502   144210    +4708     
  Branches      15299    16433    +1134     
============================================
+ Hits         102638   107003    +4365     
+ Misses        28908    28748     -160     
- Partials       7956     8459     +503
Flag Coverage Δ
inttests 26.62% <50.00%> (+2.03%) ⬆️
systests 23.11% <50.00%> (-1.21%) ⬇️
unittests 73.70% <50.00%> (+0.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 79.41% <50.00%> (+0.11%) ⬆️

... and 1055 files with indirect coverage changes

@lhotari
Copy link
Member Author

lhotari commented Mar 5, 2025

Closing this PR since @poorbarcode will create a new PR addressing the root cause.

@lhotari lhotari closed this Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poorbarcode poorbarcode poorbarcode left review comments

@nodece nodece Awaiting requested review from nodece

@dao-jun dao-jun Awaiting requested review from dao-jun

@shibd shibd Awaiting requested review from shibd

Assignees

@lhotari lhotari

Labels
doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.11 release/3.3.6 release/4.0.4
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lhotari @codecov-commenter @poorbarcode